validateChecksum() uses a truthy check to detect unexpected trailers on line 56. If setExpectedChecksum is called with an empty string value (e.g. from a malformed trailer like x-amz-checksum-crc32: with nothing after the colon), the check evaluates to false and the unexpected trailer goes undetected. Consider using this.trailerChecksumValue !== undefined for consistency with line 27. — Claude Code

storeObject.js passes checksumedStream.stream (a ChecksumTransform) to data.put, but ChecksumTransform never has .headers set on it. In the old code, prepareStream always returned a stream with .headers = stream.headers. If any data backend or middleware reads .headers from the stream passed to data.put, this will be undefined.

I checked lib/data/wrapper.js and it does not reference .headers, but worth verifying against all backend implementations (sproxyd, azure, gcp, etc.) that none rely on it.

— Claude Code

.headers: checked — no backend in lib/data/ or Arsenal reads .headers off the stream passed to data.put.

Potential race: if ChecksumTransform errors during _flush (e.g. crc64nvme digest fails), the error goes through errCb which calls cbOnce. But if writableFinished was false, the doValidate listener is also registered on 'finish'. Since _flush called callback(error), the stream will not emit 'finish' so the listener is harmless — but the 'finish' and error paths are independent, and both can potentially reach cbOnce. The jsutil.once(cb) guard prevents double-callback, but the upgraded onStreamError (line 88-96) could trigger batchDelete even though doValidate also triggers batchDelete on checksum failure. Consider whether both paths could fire in an edge case.

— Claude Code

double-delete: not a risk — finish and error are mutually exclusive in the Node.js stream lifecycle, so both handlers can't fire for the same stream.

Test names using itSkipIfAWS() in makeScenarioTests start with should, but the dynamically generated tests in the for loops (e.g. line 472 below) don't. They should follow the same convention.

— Claude Code

Test names passed to itSkipIfAWS() (which wraps it()) should start with should per project convention.

— Claude Code

It could be even cleaner with an FSM when state management becomes a bit more complex, by maintaining an explicit state variable and acting based on its value.

Nitpick suggestion for clarity:

• add const checksumHeader = checksumHeaders[0];
• thereafter, check if a header is present via if (checksumHeader) and use it directly, instead of checking the count and using checksumHeaders[0]

I know indexing an empty array does not throw in JS, but I don't feel comfortable doing it

You can do:

const checksumHeader = xAmzChecksumCnt > 0 ? checksumHeaders[0] : undefined;

My point was more about using a separate variable since we have zero or one checksum, it clarifies the logic.

done

done

You can probably assume that it's a Buffer, I don't think we would send string contents to this stream in any case.

I had issues in the past with encoding, so I prefer to be explicit and always check

Since input is a buffer, encoding will be ignored, so better leave it out (if you wanted to support strings, then you would have to pass encoding to the Buffer.from call above, but as I said I don't think we would have to deal with strings).

True, removed the encoding argument

I feel like whether the checksum comes from the checksum header or a trailer, they should be treated the same way when it comes to comparing with the computed checksum.
I understand that you defer validation of the checksum value itself to validateChecksum, but maybe it would be best to validate it directly here, then update this.expectedChecksum so the following logic of validateChecksum can be agnostic of where the checksum comes from. (If you really need to defer the error reporting to validateChecksum, you could set an error flag this.checksumError or similar and check it at the beginning of validateChecksum, but it may not be needed)

The thing is that when we call setExpectedChecksum we don't know if _flush has been called and finished, so this.digest may not be ready. In dataStore we make sure the checksumed stream is finished before calling validateChecksum, so we know this.digest is ready.

Let me know if this responds your concern or if I missed something

What I meant is that every sanity check done in the this.isTrailer case (https://github.com/scality/cloudserver/pull/6112/changes#diff-5ea0da6c64111c8a94b664848cc7c8e9201b530aa77f0a6e962a905af8055f9eR25) except the final digest check could be done within setExpectedChecksum, where the function would:

• on success, set this.expectedChecksum to be validated by validateChecksum at the end, i.e. using the same code path than for non-trailer checksum
• on error, either return a validation error immediately or mark the error in an attribute this.checksumError (for example) to be checked and returned first thing by validateChecksum

I understand, but if we return an error then we need to handle it in the trailer event handler not great, and setting a variable and then checking it in validateChecksum is not a improvement in my opinion

I don't think you benefit from creating and overwriting this temporary variable, you could just use request or middleware streams like v4Transform directly where stream is used.

removed

done

The test is marked as "checksum validation passes" but it seems there is no checksum passed at all, is it meant to pass a valid x-amz-checksum-crc32 header matching the hello world payload for example?

fixed, added checksum

nitpick on the variable name wrongDigest: technically this function doesn't care if the digest is correct or not, so it could just be called digest (meaning it could also potentially be used to build correct bodies)

fixed with refactor of trailer body creation

fixed with refactor of trailer body creation

Could be worth adding some tests with larger synthetic bodies (e.g. 10MB of As), both with correct and wrong checksum, to check that streaming checksumming works.

done

If you don't use customizeDescription you can use errors to have a new instance with the current stacktrace, if passing an error with a fresh stacktrace matters.

Otherwise you'd have a pre instanciated error without .stack trace. https://github.com/scality/Arsenal/blob/362f7908b6126709add96ee981d5235d96bab58c/lib/errors/index.ts#L196-L214

done

Should we rather use maximumAllowedUploadSize ? It's for PutObject, even if it's the same value, the maximumAllowedPartSize is for MPU (with lower value for MPU_TESTING)

this transform is used for both cases

this can be inside the above top-level describe, to keep only 1 top-level describe

done

nitpick: trailer -> trailing

done

trailer -> trailing

done

just to make it more explicit

done

same idea

done